-
Notifications
You must be signed in to change notification settings - Fork 612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
E2E: Add ICA MsgSubmitTx tests (success + failure) #2021
E2E: Add ICA MsgSubmitTx tests (success + failure) #2021
Conversation
https://github.com/cosmos/ibc-go into cian/issue#1911-support-backwards-compatibility-tests
…terchain-accounts-sample-test
return err | ||
} | ||
|
||
func (s *InterchainAccountsTestSuite) TestMsgSubmitTx_SuccessfulTransfer() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ive got the fail case /unhappy case below as well, so maybe we can keep this naming but break out the unhappy case into a separate test function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separated out into 2 tests as you suggested!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Excellent work
_, err = chainB.GetBalance(ctx, hostAccount, chainB.Config().Denom) | ||
s.Require().NoError(err) | ||
|
||
expected := testvalues.IBCTransferAmount + testvalues.StartingTokenAmount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it a little confusing that we use IBCTransferAmount
in a bank send. Maybe we could add a short comment explaining we use that var in the bank send. I don't have a strong preference though
e2e/interchain_accounts_test.go
Outdated
} | ||
|
||
// RegisterICA will attempt to register an interchain account on the counterparty chain. | ||
func (s *InterchainAccountsTestSuite) RegisterICA(ctx context.Context, chain *cosmos.CosmosChain, user *ibctest.User, fromAddress, connectionID string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer RegisterInterchainAccount
to be explicit.
We could refactor this function to take the MsgRegisterAccount
as an arg in place of the fromAddr
and connnectionID
. Then we can built the message with an explicit version for both incentivized and non-incentivized channels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea makes sense
e2e/interchain_accounts_test.go
Outdated
transferMsg := &banktypes.MsgSend{ | ||
FromAddress: hostAccount, | ||
ToAddress: chainBAccount.Bech32Address(chainB.Config().Bech32Prefix), | ||
Amount: sdk.NewCoins(testvalues.DefaultTransferAmount(chainB.Config().Denom)), | ||
} | ||
|
||
// assemble submitMessage tx for intertx | ||
submitMsg, err := intertxtypes.NewMsgSubmitTx( | ||
transferMsg, | ||
connectionId, | ||
controllerAccount.Bech32Address(chainA.Config().Bech32Prefix), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming nits: I think we should name vars to correspond with the msg type. I got confused with transferMsg
as when I read it I think ibc transfer (ics20).
What do you think about using msgSend
and msgSubmitTx
, I find it reads a little easier.
|
||
// setup 2 accounts: controller account on chain A, a second chain B account. | ||
// host account will be created when the ICA is registered | ||
controllerAccount := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about renaming controllerAccount
to ownerAccount
and the hostAccount
as interchainAccount
?
cc: @damiannolan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i personally think that this change would be more confusing -- but that's just my opinion because of the language from the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like host/controller terminology
e2e/interchain_accounts_test.go
Outdated
s.Require().Equal(len(channels), 2) | ||
}) | ||
|
||
t.Run("execute bank transfer over ICA through controller account", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.Run("execute bank transfer over ICA through controller account", func(t *testing.T) { | |
t.Run("interchain account executes a bank transfer on behalf of the corresponding owner account", func(t *testing.T) { |
e2e/interchain_accounts_test.go
Outdated
|
||
t.Run("execute bank transfer over ICA through controller account", func(t *testing.T) { | ||
|
||
t.Run("fund host wallet", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.Run("fund host wallet", func(t *testing.T) { | |
t.Run("fund interchain account wallet", func(t *testing.T) { |
e2e/testconfig/testconfig.go
Outdated
@@ -8,6 +8,7 @@ import ( | |||
) | |||
|
|||
const ( | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits. Nice work :-)
Description
This PR adds an E2E test which tests the happy path for in interchain accounts transaction using the
icad
docker images.Thank you @charleenfei and @damiannolan for all the help with this one 🥇
closes: #1941
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes